docs: RFC to formalize node termination contract - #3256
Conversation
kubernetes-prow
Bot
added
the
do-not-merge/invalid-commit-message
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ketanjani21 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
cncf-cla: yes
kubernetes-prow
Bot
added
the
size/L
|
|
||
| Node termination is one of Karpenter's oldest and most trafficked flows. Multiple entry points funnel into it — user `Node`/`NodeClaim` deletion, expiration, disruption (consolidation, drift), and health-driven forceful termination — and the same set of controllers coordinate through implicit, code-only conventions to drain pods, wait for volumes to detach, and terminate the underlying cloud instance. There is no single place that names the flow's inputs, its status-condition progression, or the guarantees it makes to callers. | ||
|
|
||
| This RFC formalizes that contract as a durable design document. It changes **no observable behavior**. It catalogs what the flow already does, sequences the status conditions it drives, enumerates its guarantees and edge cases, and disambiguates the two ways the `karpenter.sh/nodeclaim-termination-timestamp` annotation gets set. Its purpose is to make the flow legible enough that termination changes stop being expensive to review and grace-period correctness bugs stop being expensive to find. |
There was a problem hiding this comment.
Hope I'm not yanking you around, but we should make karpenter.sh/nodeclaim-termination-timestamp a validated field in the status of the nodeclaim similar to #2889
There was a problem hiding this comment.
Thank you for the feedback and not yanking me around at all, this is very helpful. I saw you closed #2889 and linked #3029 there
On the open question you raised on #2889 ("if terminationTimestamp can be extended or if can only be reduced"): the RFC already answers this in the "Invariant (write ordering)" section. The annotation can only get tighter, never looser. nodeclaim/lifecycle skips its write if the annotation is already set; node/health refuses to overwrite an existing past-time value. So the deadline only monotonically tightens (can be reduced, cannot be extended).
Reading your comment as an ask to fold the field migration into this RFC:
- Add
Status.TerminationTime *metav1.Timeon NodeClaim, matching feat: surface nodeclaim termination timestamp in status #2889's shape. - Both
nodeclaim/lifecycleandnode/healthwrite the field alongside the existing
annotation. - Downstream consumers (terminator, health controller) read the field.
- Validation: kubebuilder
+kubebuilder:validation:Format=date-time. Monotone-tighten
enforced by the writing controllers per the RFC's existing invariant; a validating webhook would be stronger but not strictly required if we trust the two writers. - Migration: annotation kept for one release for backward compat, then removed.
Will fold this in and reference #2889 as prior art. Should the RFC cover the annotation
deprecation timeline too, or leave that to a follow-up graduation PR? Once the RFC is
accepted, do you want the implementation carried on the same PR, or a separate
implementation PR?
f537508 to
6cd48f5
Compare
Coverage Report for CI Build 32905365553Coverage remained the same at 83.025%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Formalizes the node termination flow's inputs, status-condition progression, guarantees, and edge cases as a durable design document. Changes no observable behavior. The termination flow has multiple entry points (Node delete, NodeClaim delete, expiration, disruption, health) converging on the same controller chain, but the contract between them lives implicitly across four packages. Reconstructing it from code has produced recurring grace-period correctness bugs (kubernetes-sigs#3032, kubernetes-sigs#3111) and adds review overhead on every termination-touching PR (kubernetes-sigs#3063). Derek's node-repair RFC (kubernetes-sigs#3192) explicitly depends on this contract as a follow-up. The RFC is intentionally scope-limited: it documents what already exists. Migrating the nodeclaim-termination-timestamp annotation to a first-class field and consolidating its two writers are named as follow-ups; both preserve rather than replace this contract. Fixes: kubernetes-sigs#3029
6cd48f5 to
cf55536
Compare
|
Invalid commit message issues detected Invalid commit messagesKeywords which can automatically close issues and hashtag(#) mentions are not allowed.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Fixes: #3029
Description
Formalizes the node termination flow as a design doc under
designs/, following the pattern set by #3192. This changes no observable behavior — it documents what already exists.Termination has multiple entry points (Node delete, NodeClaim delete, expiration, disruption,
node/health) converging on the same controller chain, but the contract between them lives implicitly across four packages (node/termination,terminator,nodeclaim/lifecycle,node/health). Reconstructing that contract from code has produced recurring grace-period correctness bugs (#3032, #3111) and adds review overhead on every termination-touching PR (see the review history on #3063). Derek's node-repair RFC (#3192) explicitly names this contract as a follow-up dependency.The RFC covers:
nodeclaim-termination-timestamp— the two writers (nodeclaim/lifecycleandnode/health), their intents, and the write-ordering invariant that makes the deadline monotone (it can only get tighter, never looser).Drained→VolumesDetached→InstanceTerminating, with reasons and transition triggers.Kept single-purpose intentionally. A companion package-level
pkg/controllers/node/termination/doc.gopointing to this RFC is proposed as a follow-up PR so a contributor grepping the code discovers the contract next to it.How was this change tested?
N/A — design document only.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
LLMs were used in the writing of this document, but the contribution is mine.